fix(ui): keep transcript geometry stable during reader input - #5192

Reviewed at exact head 6a8e418. No P0/P1/P2 code findings; approval is based on the verified transcript geometry, publication identity, and navigation boundaries.

Posting one confirmed finding now rather than holding it for a full synthesis, at the author's request. This is not an approval and not a complete review — a multi-reviewer pass on 61d1b7fcbbf834ad90f1f7962f12e9c8a25c1b2e is still in progress, and I will report the rest when the other lanes return.

The finding is inline. It was introduced at 6a8e418fec94dbc3841be659ea8b093ea6126941, the author reproduced it independently, and the fix at 61d1b7fcb is in this head. I verified the defect on the old head, the fix on the new head, and the new regression test by ablation — each with its own control, so the inline comment is a record of a closed issue rather than an open one.

Two things I checked that are not findings, recorded so they are not re-derived:

• The cold-start Markdown transition is disclosed accurately. The description states that the plain-text-to-Markdown swap can move the anchor by 14.5px, that this PR does not resolve it, and that the fixed-content assertions do not claim to cover it. The history-at-top regression waits for fonts and the initial Markdown module before recording its anchor, so that transition sits outside what the suite observes. That is a real gap, but it is a declared one, and the tests do not overstate their reach.
• The performance evidence is also disclosed accurately. The post-change run is on pre-rebase commit 21aa204a5 and the description says so, along with the caveat that passing the measurement job is not a statistical verdict. So there is currently no performance measurement bound to this head. Whether that gap blocks merge is a maintainer decision, not something a few milliseconds either way could settle.

Still outstanding from my side: the architecture/simplicity lane, the #5170 integration and race lane, and independent cross-verification of the finding above. I will post those separately.

Automated review notice: This comment was posted by an automated review agent operated by jackwener (seat: kabi-opus). It is not an independent human review and does not replace one.

Focused re-review at 7cc88880620f012f0d5bf72b1537915d6f3c045b. I have reviewed earlier revisions of this pull request and raised some of the issues discussed below, so this is not an independent first reading. I am not approving, for a reason I state at the end.

The rebase's combined semantics hold where I checked them. Two places worried me and neither turned out to be a defect:

• In the WorkHub controller the publication commit settles buffered steps with for (const message of settled) if (next) next = settleLiveTurnBufferStep(next, message.id);, and that helper can return undefined, which would silently skip the remaining settlements. Reading the shared buffer helper, undefined is produced only when every retained Turn has been dropped, so an early exit means the buffer is already empty and there is nothing left to settle. The loop is correct.
• The tool row now carries both the upstream observation flag and this change's transcript boundary, and the boundary attribute is written with an empty value rather than a named one. The only rule that consumes it matches on attribute presence, and every call site writes it the same way, so the combination is consistent.

The buffer reconciliation the rebase adopted also settles a terminal Turn's steps directly from durable assistant messages, with the stated intent of finishing a handoff whose original callback was lost. That is a useful safety net for precisely the class of stranded live step this change's earlier revisions had to reason about carefully.

The input lifecycle unification is real, not cosmetic. Scroll end now calls the idle notification before its early return, with a comment explaining that an explicit navigation may already have retired the gesture while a pointer or touch was still held, so a release must still wake publication. Pointer release clears the pointer first and then goes through that same entry. The active-input test is defined once and reused by the notifier, the public snapshot and the commit guard, rather than repeated inline. The partial multi-touch release now returns early while any touch remains.

Earlier review items, checked one by one rather than assumed closed.

• Stranded pending publication after releasing the pin with the scrollbar held — closed. The release path reaches the idle notification unconditionally.
• Stale render read after the await in the durable handoff — closed. The published-message question is now answered by the publication owner against its live reference rather than by a predicate closed over a render's array.
• Partial multi-touch release treated as full settlement — closed, by deriving settlement from the remaining touch count.
• Unify the active-input predicate — closed.
• Avoid selecting with a mutating predicate in the WorkHub controller — not addressed. The settled set is still consumed by calling delete inside a filter predicate, and the suggested helper does not exist. This was raised as non-blocking and remains non-blocking; I note it only because the closure of the other items makes this one easy to lose track of.

One earlier concern is now genuinely pinned, which is worth recording. The publication query depends on object identity rather than message id, and in earlier revisions that invariant was carried only by a comment. A test now asserts that a structurally identical clone carrying the same id is not treated as published, so changing the query to an id or structural comparison would fail a test rather than pass silently.

Why I am not approving. The request named six areas. I covered the input, pin and idle lifecycle; the reachable races the two most recent commits address; the rebase's combined semantics; and the outstanding-comment closure above. I did not audit window fill and trim against publication commit timing, and I did not assess the test suite for duplication or flakiness beyond the files these changes touch. Approving on this coverage would mean presenting a partial review as a complete one. I would rather say plainly what I checked and leave the approval to a reviewer who covers the rest, or to a later pass of my own.

Verification limits. I read code and hosted results only; I ran no tests, build, Storybook smoke or Electron session, so I did not observe any of this behaviour executing and did not measure timing windows. The author's local run counts are not part of my evidence. On this commit the hosted test and windows_recovery checks are terminal and successful. I did not evaluate performance: by the stated acceptance bar there is no clear regression evidence to weigh, and I neither produced nor require a speedup measurement.

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

Re-review at f4bfcb300dff630491db01c461f579aaa4258a66, covering the test-integrity question I had previously left uncovered.

The removal is correct, and for a stronger reason than the one given. The stated reason is that input.frames depends on frame timing that nothing guarantees. Reading the probe confirms that: the counter only increments on animation frames that land while a wheel has been seen and recording has not yet been armed, so whether any frame falls in that window is a scheduling accident.

The part worth adding is what that implies for the other three assertions removed alongside it. heightDrift, rangeChanges and reverse are accumulated inside the same guarded block. When no frame lands in that window they are never written, so they keep their initial zeros and their assertions — height drift within one pixel, no membership change, no reversal — all pass without observing anything. input.frames > 0 was not a fourth assertion beside them; it was their positive control.

That makes removing all four together the right call rather than a loss of three good checks. Keeping them without a dependable control would leave three assertions that can pass on zero samples, which is worse than not having them: a green run would then carry no information about the property it names. This file already applies that discipline elsewhere — the displacement assertion is guarded by requiring that boundaries were observed and that at least one carried a Turn across — so this is consistent with its own standard.

What is genuinely no longer covered, stated precisely. The retained native held-thumb test does assert that height stays constant and resident membership does not change while the input is held, and it carries its own controls that the pointer interaction actually occurred. That protection is real. It exercises the pointer path, though, while the removed sampling exercised the wheel path. In the scroll authority these are distinct branches that open the same publication gate by different means — a wheel establishes a gesture, a held thumb sets the pointer. So "the height and membership assertions are still there" is accurate for held-thumb input and not a like-for-like replacement for wheel input.

I am recording that as a coverage observation rather than a defect, and I would not hold the change for it. The gate those assertions ultimately protect is shared, the wheel path retains the boundary-displacement, boundedness and return-to-tail assertions in the same test, and the fixed-document story still drives wheel events with its own motion assertions. The narrowed claim is that no end-to-end test now asserts in-input geometry specifically for wheel input.

On the rest of this change. The increment since the previously reviewed head is this deletion and nothing else — no production code is touched — so my earlier findings on the session and renderer surface stand unchanged, including the range-controller handoff verdict and the retirement verdict I gave there.

Why I am still not approving. Two of the six areas I was asked to cover remain outside my review: window fill and trim against publication commit timing, and duplication across the test suite beyond the files these changes touch. This comment closes part of the second area — the flakiness question for this specific probe — but not the rest of it. The hosted test check was also still running when I posted, so I make no statement about it.

Verification limits. I read code and hosted results only and ran nothing. I did not reproduce the frame-timing behaviour described in the removal rationale; I verified that the code makes it possible, not that it is what happened in the failing run.

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

Not approving at f4bfcb300dff630491db01c461f579aaa4258a66. The required check is terminal and failing, and the failure is a combination regression: each side is green alone, the rebase of one onto the other is not.

The two areas I previously listed as uncovered are now covered, and neither produced a defect.

Window fill and trim against publication commit timing. The fill and trim mechanism is untouched by this change, so the question is only how it composes with deferred publication. It composes correctly: trimming mutates the range store, the rendered transcript reads published state rather than the store, and publication passes through the same idle gate as everything else. A trim that lands while the reader holds an input therefore cannot move anything; it becomes visible when the gate opens, and that moment is what the retained boundary-displacement assertion measures — with its own controls requiring that boundaries were seen and that at least one carried a Turn across. Paging is not a second authority writing geometry.

Duplication within the change's scope. The four unit tests added here and the one end-to-end test retained in the same file are complementary rather than overlapping. The unit tests pin input-lifecycle transitions — settlement across multiple touch contacts, a stationary held scrollbar, an edge wheel whose scrollend never arrives, and navigation during a hold — which need deterministic control of event ordering. The end-to-end test covers real Host paging, real layout displacement, boundedness and return-to-tail, which a fake DOM cannot establish. Each of those four corresponds to a defect found earlier in this pull request's review history, pinned at the cheapest layer that can hold it deterministically. This also completes the point I made about the removed wheel sampling: that protection did not disappear, it relocated to a layer that can carry it without depending on frame scheduling.

The blocker. Storybook smoke fails on this commit:

[product-model-picker--existing-conversation (light/default)]
expect(element).toHaveTextContent()
Expected element to have text content:
  切换模型可能需要重建服务商提示缓存，使下一次请求更慢或成本更高。
Received:

The model-switch warning for an existing conversation renders empty.

Why I am calling this a combination regression rather than a pre-existing failure. I checked each side separately:

Both inputs are green on their own and the result is red. The only change since the last head where this step actually ran to completion is the rebase range; the most recent commit removes end-to-end sampling and touches no production code, and the step was skipped on the intermediate head because an earlier step failed first. So this is not a pre-existing condition and not caused by the last commit.

A lead worth checking first, offered as a lead and not as a proven cause. In the shell, the flag behind this warning falls back to the durable session summary precisely so that a cleared or not-yet-published transcript cannot hide it, and the comment beside it says so. The WorkHub call site derives the same flag from the published message count alone, with no such fallback. Deferred publication is this change's central mechanism, so a path that reads only published messages is where I would look first for a warning that renders empty. I have not traced which path this story renders, and I have not reproduced the failure, so this is where to start rather than a diagnosis.

Everything else on my surface is unchanged and still stands — the identity and rows handoff, the range-controller verdict, and the retirement verdict, all re-confirmed at this commit.

Verification limits. I read code and hosted results only and ran nothing locally. I did not reproduce the Storybook failure, did not identify the specific commit in the rebase range that introduces it, and did not verify the mechanism suggested above.

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

Approve at 15b0e988d1cd0d11d36f0aef5092eb23e3e91fb9.

The blocker I raised is resolved, and the remedy is the right kind. I reported that Storybook smoke failed here while both main and this branch before its rebase were green. The increment since then waits for the restored trigger to actually hold focus before sending the key, instead of sleeping or loosening anything. The assertion that failed — that the switch warning appears — is unchanged, and the assertions before it, that nothing announces prematurely, are unchanged too. That is a precondition fix: asserting the effect of a keystroke is meaningless if the keystroke never reached its target, which is what the empty announcement was telling us.

I should correct one thing I wrote. Alongside that report I offered a candidate mechanism — a call site deriving conversation history from published messages without the durable fallback its sibling has — and labelled it a lead rather than a diagnosis. It was not the cause. The observation held: green on each side, red on the combination, with the intermediate head's step skipped rather than passing. The explanation was a focus race in the story that the rebase's timing exposed, not a behavioural regression. I would rather say that plainly than leave a wrong lead standing next to a correct observation.

Coverage, and what I verified myself rather than quoted.

• Input, pin and idle lifecycle. Scroll end reaches the idle notification unconditionally, before its early return, so a release wakes publication even when an explicit navigation already retired the gesture. Pointer release clears the pointer and then goes through that same entry. The active-input test is defined once and reused by the notifier, the public snapshot and the commit guard. Partial multi-touch release settles only when the last contact ends.
• Reachable races across async waits. The published-message question is answered by the publication owner against its live reference rather than by a predicate closed over a render's array. That the query depends on object identity rather than message id is now pinned by a test asserting that a structurally identical clone with the same id is not treated as published — so changing it to an id comparison fails a test instead of passing silently.
• Window fill and trim against publication commit timing. Trimming mutates the range store; the rendered transcript reads published state; publication passes the same idle gate. A trim landing while an input is held therefore cannot move the reader, and becomes visible when the gate opens — the moment the retained boundary-displacement assertion measures, with its own controls requiring that boundaries were observed and that at least one carried a Turn across. Paging is not a second authority writing geometry.
• Tests protecting real behaviour, and duplication. The four unit tests added here and the end-to-end test retained beside them are complementary: the unit tests pin input-lifecycle transitions needing deterministic event ordering, the end-to-end test covers real Host paging, real layout displacement, boundedness and return to tail. Each of the four corresponds to a defect found earlier in this pull request's own review history, pinned at the cheapest layer that can hold it. The earlier removal of frame-dependent wheel sampling fits the same pattern: those assertions were only meaningful while their frame counter was non-zero, so that counter was their control rather than a fourth assertion, and the protection relocated rather than disappearing.
• Earlier review items. Re-checked one by one rather than assumed: the stranded publication after releasing the pin with a scrollbar held, the stale render read after the durable handoff's await, the partial multi-touch settlement, and the request to unify the active-input predicate are all closed. One non-blocking nit is not addressed — the settled set is still consumed by calling delete inside a filter predicate — and it was raised as non-blocking by its author.

Limits of this approval. I read code and hosted results only; I ran no tests, build, Storybook or Electron session, so nothing here rests on my own execution. The author's local runs are not part of my evidence. On performance I make no claim in either direction: there is no clear regression evidence to weigh on this commit, and I neither produced nor require a speedup measurement. This approval also rests on a single reviewer under one owner — the other seats approached for this change were unavailable, so the cross-owner independence sought for it was not achieved, and that gap is a fact about this approval rather than something it resolves.

The required checks are terminal and successful on this commit, and it is mergeable. No merge was performed.

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.